Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rbd: add support for rbd_diff_iterate3 api #1064

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

rbd: add support for rbd_diff_iterate3 api #1064

wants to merge 1 commit into from

Conversation

Rakshith-R
Copy link
Contributor

This commit adds support for rbd_diff_iterate3
through DiffIterateByID() which performs
diff interate using snapshot id instead of
snapshot name.

This is useful in cases such as when rbd snapshot is in trash or rbd(group) snapshot in in group namespace.

refer:

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

@Rakshith-R
Copy link
Contributor Author

hey,

Please take a look at the function name and the way it is defined.
I have yet to add tests.
I'll add them after we have finalized the structure of the call.

The new api will be added in this pr https://github.com/ceph/ceph/pull/60844/files#diff-b4983d99df0348718f7db5606f7ee3ca00fb0e7a87b4a69740f7ada6dd283856

@nixpanic @ansiwen @phlogistonjohn @anoopcs9

Thanks,

@Rakshith-R Rakshith-R mentioned this pull request Jan 29, 2025
Open
14 tasks
idryomov
idryomov reviewed Jan 29, 2025
View reviewed changes
rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved
@Rakshith-R
rbd: add support for rbd_diff_iterate3 api
402c41b 
This commit adds support for rbd_diff_iterate3
through DiffIterateByID() which performs
diff interate using snapshot id instead of
snapshot name.

This is useful in cases such as when rbd snapshot is
in trash or rbd(group) snapshot in in group namespace.

refer:
- https://tracker.ceph.com/issues/65720
- ceph/ceph#60844

Signed-off-by: Rakshith R <[email protected]>
@phlogistonjohn
Copy link
Collaborator

@Rakshith-R please take a look at the test failures as they seem related to the change. Thanks!

@Rakshith-R
Copy link
Contributor Author

@Rakshith-R please take a look at the test failures as they seem related to the change. Thanks!

Yes, the tests are expected to fail for now.
The rbd_diff_iterate3 call needs to be merged upstream ceph first in ceph/ceph#60844 .
I'll rebase the pr once ceph's pr going in.

I Just added the pr early so I can get feedback on naming conventions and format.

@phlogistonjohn
Copy link
Collaborator

phlogistonjohn commented Feb 7, 2025 •
edited
Loading

OK, as a general request - not specifically to you but to all contributors that are adding APIs to go-ceph that do not have a functioning versions in ceph - please make it very clear up front that the work is not complete in ceph. (Edit: a sentence or two in the descrition is good, even better if it includes links to Issues, PRs or design docs)

In addition, I would like to see PRs of this sort be in draft status and also consider using the ceph-libs label. Draft tells us that the PR is not in a mergable state - it does not mean that we won't discuss it or provide feedback (although explicit requests for feedback are welcome on draft prs). the ceph-libs label, if you look at the description, indicates that something is needed on the ceph side.

Today, I'll make these changes to the PR after I submit this comment. In the future please consider these suggestions for PRs like this that can't be merged because work in ceph is still pending. Thanks!

@phlogistonjohn phlogistonjohn marked this pull request as draft February 7, 2025 13:21
@phlogistonjohn phlogistonjohn added ceph-libs Fix or enhancement needed in ceph libraries API This PR includes a change to the public API of a go-ceph package labels Feb 7, 2025
@Rakshith-R
Copy link
Contributor Author

OK, as a general request - not specifically to you but to all contributors that are adding APIs to go-ceph that do not have a functioning versions in ceph - please make it very clear up front that the work is not complete in ceph. (Edit: a sentence or two in the descrition is good, even better if it includes links to Issues, PRs or design docs)

In addition, I would like to see PRs of this sort be in draft status and also consider using the ceph-libs label. Draft tells us that the PR is not in a mergable state - it does not mean that we won't discuss it or provide feedback (although explicit requests for feedback are welcome on draft prs). the ceph-libs label, if you look at the description, indicates that something is needed on the ceph side.

Today, I'll make these changes to the PR after I submit this comment. In the future please consider these suggestions for PRs like this that can't be merged because work in ceph is still pending. Thanks!

Noted, I'll keep these points in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idryomov idryomov idryomov left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
API This PR includes a change to the public API of a go-ceph package ceph-libs Fix or enhancement needed in ceph libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rakshith-R @phlogistonjohn @idryomov